Skip to content

chore(rendering): remove the Clamp Lighting effect - #85

Open
soloturn wants to merge 1 commit into
developfrom
soloturn-remove-clamp-lighting
Open

chore(rendering): remove the Clamp Lighting effect#85
soloturn wants to merge 1 commit into
developfrom
soloturn-remove-clamp-lighting

Conversation

@soloturn

@soloturn soloturn commented Aug 19, 2026

Copy link
Copy Markdown
Contributor

Part of MovingBlocks/Terasology#5152.

Clamp Lighting was reported to visibly do nothing. It clamps this pass's output color to [0.0, 1.0] before later HDR-aware post-processing (bloom, eye adaptation, tone mapping) runs on it. Most framebuffer formats already clamp on write regardless, so the setting only has a visible effect in specific HDR configurations - which is why toggling it did nothing for most players in practice. The maintainer who investigated asked for outright removal rather than a fix: "remove clamplightning in both ui and Rendering Config."

Drops the #if defined(CLAMP_LIGHTING) branch, keeping the unclamped path - the engine's default.cfg already had the setting off by default, so this is the behaviour every player already saw. Also drops the doc section and its screenshot, the only other reference to the setting in this repo.

Companion PR

The config/UI side lives in the engine repo: MovingBlocks/Terasology#5370

MovingBlocks/Terasology#5152: Clamp Lighting was reported to visibly do
nothing. Investigated and confirmed: it clamps this pass's output color
to [0.0, 1.0] before later HDR-aware post-processing (bloom, eye
adaptation, tone mapping) runs on it. Most framebuffer formats already
clamp on write regardless, so the setting only has a visible effect in
specific HDR configurations - not a bug, a setting whose effect never
showed up where a player toggling it would look. The maintainer who
looked into it asked for outright removal rather than a fix:
"remove clamplightning in both ui and Rendering Config."

Dropped the #if defined(CLAMP_LIGHTING) branch, keeping the unclamped
path - the engine's default.cfg already had it off by default, so this
is the behaviour every player already saw. Also drops the doc section
and its screenshot, which was the only other reference to the setting in
this repo.

Companion change removing the config/UI side in the engine repo.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Aug 19, 2026

Copy link
Copy Markdown

Important

  • 🔍 Trigger review

This repository does not receive automatic reviews because it has fewer than 10 stars.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: d8c63863-d1a1-48c9-93e9-1e89115c18ec


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@soloturn

Copy link
Copy Markdown
Contributor Author

Ran a visual/log test with this change (plus the rest of a larger merge-train batch) using the full Omega module set, then compared its CoreRendering render-graph (DAG) log output against an unmerged develop baseline built the same way.

Both runs produce the exact same 231 RenderGraph/AbstractNode warnings and errors (node-connection wiring messages like "trying to disconnect two nodes that aren't connected," "input connection returned null," "overwrites data of existing connection") - identical set of messages, same node names (backdropNode, ambientOcclusionNode, opaqueBlocksNode, deferredMainLightNode, etc.), only differing in interleaving order between the two runs (normal HashMap/iteration-order variance, not a content difference).

So this DAG noise is pre-existing in develop, not introduced by this clamp-lighting removal. Flagging it here since it was a plausible suspect (this PR removes a pipeline stage's shader/config wiring) but the baseline comparison clears it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant